-
-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update dependency for halo2 MsmAccel trait #377
Conversation
Replaces the dependency for the MSM acceleration layer to use the version from the Privacy Scaling Explorations repo. Still needs to depend on `halo2curves` explicitly, as `halo2_middleware` does not reexport it.
@@ -22,7 +23,7 @@ rand_xorshift = "0.3" | |||
rayon = "1.8" | |||
|
|||
# In CI "asm" needs to be disabled as some agents don't support ADX. | |||
halo2curves = { git = 'https://github.com/taikoxyz/halo2curves', branch = "pr-pse-exec-engine" } | |||
# halo2curves = { git = 'https://github.com/taikoxyz/halo2curves', branch = "pr-pse-exec-engine" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what should happen with this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from when I enabled multicore and asm features for benching (and disabled it in CI) See: 231ccc7#diff-86e33129ff3e1c7ef4e097a64b3bba9f931352847b95fa0d7c9033665c10b8b6R26
It can be deleted with.
Note that if upstream is changed to reexport part of halo2curves, we might need it again for the benchmarks:
constantine/constantine-rust/constantine-halo2-zal/benches/msm.rs
Lines 18 to 21 in 12f5686
use halo2curves::bn256::{Fr as Scalar, G1Affine as Point}; | |
use halo2curves::ff::Field; | |
use halo2curves::msm::best_multiexp; | |
use halo2curves::zal::MsmAccel; |
at least the best_multi_exp part.
@@ -10,7 +10,8 @@ repository = "https://github.com/mratsim/constantine" | |||
[dependencies] | |||
constantine-sys = { path = "../constantine-sys" } | |||
constantine-core = { path = "../constantine-core" } | |||
halo2curves = { git = 'https://github.com/taikoxyz/halo2curves', branch = "pr-pse-exec-engine" } | |||
halo2_middleware = { git = 'https://github.com/privacy-scaling-explorations/halo2' } | |||
halo2curves = { version = "0.6.1", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to depend explicitly on 0.6.1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the way cargo works, this means a dependency on 0.6.x: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
But I think the elliptic curves and CurveAffine should be reexported by Zal or Middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that clears up a lot, thanks!
This can be updated now that privacy-scaling-explorations/halo2#323 is merged upstream |
Removed the explicit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Replaces the dependency for the MSM acceleration layer to use the version from the Privacy Scaling Explorations repo (https://github.com/privacy-scaling-explorations/halo2/)
Still needs to depend on
halo2curves
explicitly, ashalo2_middleware
does not reexport it (and currently I didn't manage to figure out how to fix that).